[Root7] A path to reducing global memory management.#18083
Draft
hageboeck wants to merge 14 commits intoroot-project:masterfrom
Draft
[Root7] A path to reducing global memory management.#18083hageboeck wants to merge 14 commits intoroot-project:masterfrom
hageboeck wants to merge 14 commits intoroot-project:masterfrom
Conversation
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
89bd358 to
4d29c60
Compare
pcanal
reviewed
Mar 20, 2025
hist/hist/src/TH1.cxx
Outdated
| Bool_t TH1::AddDirectoryStatus() | ||
| { | ||
| return fgAddDirectory; | ||
| return !ROOT::ROOT7MemoryManagement() && fgAddDirectory; |
Member
There was a problem hiding this comment.
Note that this flag is primarily about file constructions (and secondarily about shared ownership). Beside the (intentional) memory leak it introduces in existing scripts, it leads most of them to not longer write the histograms to the file (because they are no longer attached to the File). It is not clear whether this kind of approach is worth it compared to a more user active change (ie. Switching explicitly from TFile to RFile).
Member
Author
There was a problem hiding this comment.
Thanks for the information!
Details will be discussed on the ROOT retreat next week.
Test Results 22 files 22 suites 3d 4h 20m 18s ⏱️ For more details on these failures, see this check. Results for commit 3878984. ♻️ This comment has been updated with latest results. |
silverweed
reviewed
Mar 21, 2025
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
4d29c60 to
1224a4b
Compare
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
b894456 to
167c0cc
Compare
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
342a3cb to
0ca4f8a
Compare
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
0ca4f8a to
68de9c4
Compare
1 task
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
68de9c4 to
9c5ce87
Compare
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
9c5ce87 to
b79032e
Compare
hageboeck
force-pushed
the
root7-memoryManagement
branch
2 times, most recently
from
74a6279 to
8d9511a
Compare
hageboeck
force-pushed
the
root7-memoryManagement
branch
2 times, most recently
from
0b52df2 to
c9f787b
Compare
hageboeck
force-pushed
the
root7-memoryManagement
branch
2 times, most recently
from
256881d to
d354941
Compare
Explicitly assign histograms to directories. In this way, the tests proceed irrespective of whether or not implicit object ownership is in use.
- Don't rely on implicit registration to directories. - Make the test insensitive to any defaults by always adding the histogram to the file.
When the implicit ownership is switched to ROOT 7 mode, the corresponding notification needs to be masked from output diffs and/or the RootTestDriver checks.
Since the ROOT 7 ownership settings affect TH1::AddDirectoryStatus, objects read from a TFile would not associate themselves with that file. This breaks TFile contracts which don't need to be broken, as a file interface that's always reading a fresh copy from disc is RFile. So, implicit ownership "off" affects whether new objects are associated to gDirectory, TH1::AddDirectoryStatus has the same effect as before (affecting both new objects and objects read from file), and RFile undoes or avoids any potential registration. Also update the documentation to better explain what this does.
- Add DisableImplicitObjectOwnership() and EnableImplicitObjectOwnership(). - Add a function to test if ownership is on or off. - Add a silentOff mode for running tests that check the program output. - Allow for setting defaults using - The environment variable ROOT_IMPLICIT_OWNERSHIP - The rootrc variable Root.ImplicitOwnership
…ived. - Couple TH1::AddDirectoryStatus to ROOT::DisableImplicitObjectOwnership. - Add a manual exception for TTree::Draw(): Histograms and profiles will still register themselves to gDirectory because the registration is an essential part of the feature.
…ect. It makes sense when a different object with the same name is registered (the old object will leak), but if the object is identical, the replacement is like a no-op.
Previously, gDirectory was used. By creating a local directory, the test has now side effects on gDirectory.
hageboeck
force-pushed
the
root7-memoryManagement
branch
from
d354941 to
3878984
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a longer-term effort to update ROOT to work both with implicit and explicit object ownership (i.e. whether THx and similar are owned by gDirectory). The branch started by switching off this ownership and fixing all failing tests. These fixes will be done in a way that the code works both with and without implicit ownership, and will be split off into other PRs to keep the review load manageable. Once that's done, the final names and implementations of the ownership-related functions will be decided here.
TODOs:
TH1::AddDirectory()as suggested in ReplaceTH1::AddDirectorybyTDirectory::TContext#21523 ?Note: No need to review until the following PRs are merged:
gDirectory, use TContext instead. #21287TH1::AddDirectorybyTDirectory::TContext#21523